Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: use gateway from go-libipfs #9588

Merged
merged 2 commits into from
Jan 31, 2023
Merged

refactor: use gateway from go-libipfs #9588

merged 2 commits into from
Jan 31, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 25, 2023 •
edited
Loading

Part of #8524

This PR is tracking my efforts to isolate the Gateway into a sub-package which will be key to solve #8524. The plan is to first isolate the gateway inside a sub-package that does not depend on anything else from Kubo. After, we can extract it to go-lipipfs.

Progress:

  • Isolate handlers
  • Remove dependencies on:
    • kubo/tracing package - might be harder, we may want to instead extract this as a module too.
      • I extracted tracing.Span into spanTrace inside the gateway subpackage. I think this should be fine as we will still be able to continue to use the tracing functionalities.
    • kubo/assets package
      • I also improved the assets package and remove the dependency for NPM. NPM was simply running a few shell scripts which were now simplified under the build.sh file. In addition, I updated the testing script to help visualize both the DAG and the Directory indexes, as well as improved the README.
  • Move all unit tests (as possible) that are gateway specific to gateway
  • Add a README.md with an example of how it can be run 😃

TODO:

cc @lidel for visibility

@hacdias
Copy link
Member Author

hacdias commented Jan 25, 2023

@lidel I think I have achieved a very good result here. Everything gateway related is completely extracted into the gateway sub-package. I made sure that the gateway package does not depend in any other part of the Kubo base code. You can see my initial comment about it. In addition, I slightly refactored the gateway assets in order to make them easier to work with. I'm happy to receive feedback on this and address it.

We may, or not, merge this PR into Kubo's basecode. Once IPNS is merged to master, I will be rebasing this PR. Then, we can either merge this in Kubo and only then perform extraction. Or, I can use this branch as the base for the extraction. it doesn't matter. Either way, this PR allows you to see how the isolated gateway code will look like.

CI: the CI is a bit flaky for unknown reasons to me. In addition the CodeQL CI thingy is flagging things that were already here, but since they were moved, it now thinks they are new issues.

@hacdias hacdias changed the title wip: isolate gateway in sub-package refactor: isolate gateway in sub-package Jan 25, 2023
@hacdias hacdias self-assigned this Jan 25, 2023
@hacdias hacdias requested a review from lidel January 25, 2023 14:14
@hacdias hacdias mentioned this pull request Jan 26, 2023
Open
lidel
lidel reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hacdias IPNS PRs are in master (#9399, #9471) – feel free to rebase and make any changes to make this ready for final review 🙏

@hacdias hacdias force-pushed the prepare-gateway-extraction branch 6 times, most recently from 6e3b941 to ad102c1 Compare January 27, 2023 10:25
@hacdias hacdias mentioned this pull request Jan 27, 2023
Merged
@hacdias hacdias changed the title refactor: isolate gateway in sub-package refactor: use gateway from go-libipfs Jan 27, 2023
@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2023

@lidel I opened a PR in go-libipfs to add the gateway code there: ipfs/boxo#65

Also updated here to use go-libipfs instead.

@hacdias hacdias requested a review from lidel January 27, 2023 11:00
@hacdias hacdias marked this pull request as ready for review January 27, 2023 11:04
@hacdias hacdias force-pushed the prepare-gateway-extraction branch 2 times, most recently from e58bc2e to 78063a8 Compare January 27, 2023 12:27
@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2023

@lidel interop is failing bc Cloudflare npm 524.

@lidel
Copy link
Member

lidel commented Jan 27, 2023 •
edited
Loading

💭 Let's park merging this until #9579 ships, so it is easier to compare branches. I'll review a bit later.

@hacdias hacdias force-pushed the prepare-gateway-extraction branch 2 times, most recently from aec46f1 to 0b1f1ac Compare January 30, 2023 13:09
@hacdias hacdias mentioned this pull request Jan 30, 2023
Merged
lidel
lidel approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and merged ipfs/boxo#65, this one looks good too.

We should merge this asap so we don't get PRs against old gateway code in this repo.
Ok to open a separate one for GO interface adjustments described in ipfs/boxo#61

@hacdias feel free to switch this PR to the revision from main branch there and merge 🙏

@hacdias hacdias enabled auto-merge (squash) January 31, 2023 09:54
@hacdias hacdias merged commit 8d3b315 into master Jan 31, 2023
@hacdias hacdias deleted the prepare-gateway-extraction branch January 31, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidel lidel lidel approved these changes

Assignees

@hacdias hacdias

Labels
None yet
Projects
No open projects
IPFS Shipyard Team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hacdias @lidel